Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
|
|
|
joaopedrodcf
left a comment
joaopedrodcf
left a comment
There was a problem hiding this comment.
Hope you guys don't mind me trying to review the PR 🙏
| }) | ||
|
|
||
| const replacementDescription = useMarkdown(() => ({ | ||
| text: (props.replacement as { description?: string }).description ?? '', |
There was a problem hiding this comment.
What do you think of using a type guard instead of relying in casting ?
There was a problem hiding this comment.
description is either undefined or a string so it can't be anything else
There was a problem hiding this comment.
i think he means this which is safer than casting:
const description = props.replacement.type !== 'preferred' ? props.replacement.description : undefined;
return {text: description ?? ''};There was a problem hiding this comment.
Yeah exactly trying to make the code more typesafe to refactors etc
| <code>{{ replacement.replacementModule }}</code> | ||
| </template> | ||
| <template #community> | ||
| <a |
There was a problem hiding this comment.
I think we should use NuxtLink but what do you think ?
There was a problem hiding this comment.
I think NuxtLink is for internal links?
There was a problem hiding this comment.
It also works for external links as far as I know
https://nuxt.com/docs/4.x/api/components/nuxt-link#external-routing
| <PackageReplacement | ||
| v-if="moduleReplacement" | ||
| :mapping="moduleReplacement.mapping" | ||
| :replacement="moduleReplacement.replacement" | ||
| /> |
There was a problem hiding this comment.
I think we always have to options parent fetchs and decides if renders or not or we put that logic inside the component and inside we have a v-if to not render html etc
Maybe you can even have a component packageReplacementIntegration that is responsible to fetch the replacement and the rendering the visual part
| }) | ||
|
|
||
| const replacementDescription = useMarkdown(() => ({ | ||
| text: (props.replacement as { description?: string }).description ?? '', |
There was a problem hiding this comment.
we should probably just have a shared function in this repo that computes the description:
function getReplacementDescription(replacement) {
switch (replacement.type) {
case 'documented':
return undefined;
default:
return replacement.description;
}
}There was a problem hiding this comment.
Should that be in module-replacements itself since it might be useful for other consumers too or just here?
| try { | ||
| const replacement = await $fetch<ModuleReplacement | null>(`/api/replacements/${name}`) | ||
| const response = await $fetch<{ | ||
| mapping: ModuleReplacementMapping |
There was a problem hiding this comment.
@danielroe iirc you said these types are inferred based on the route. is that true? does this not infer them because of the interpolation?
| replacements: readonly(replacements), | ||
| noDepSuggestions: readonly(noDepSuggestions), | ||
| infoSuggestions: readonly(infoSuggestions), | ||
| replacements, |
There was a problem hiding this comment.
are these still readonly? was that dropped on purpose?
There was a problem hiding this comment.
as far as I can see seems by mistake,
| (event): { mapping: ModuleReplacementMapping; replacement: ModuleReplacement } | null => { | ||
| const pkg = getRouterParam(event, 'pkg') | ||
| if (!pkg) return null | ||
| const mapping = all.mappings[pkg] |
There was a problem hiding this comment.
is this a security problem? could i pass pkg as "constructor" or something funky?
| "example": "Example:", | ||
| "native": "This can be replaced with {replacement}, available since Node {nodeVersion}.", | ||
| "simple": "The {community} has flagged this package as redundant, with the advice: {replacement}.", | ||
| "native_no_version": "This can be replaced with {replacement}.", |
There was a problem hiding this comment.
| "native_no_version": "This can be replaced with {replacement}.", | |
| "native_no_version": "This package can be replaced with {replacement}.", |
5 participants
Module replacements v3
Simple
Native
Documented
Removal
Compare page